May we consider a local method variable for Message[] instead of else branch with the same method call?

I mean something like:

Message[] messages = filteredMessages;
if (this.headerMapper == null && (this.autoCloseFolder || this.simpleContent)) {
    messages = new Message[filteredMessages.length];
    ...
}
setMessageFlagsAndMaybeDeleteMessages(messages);

I was wanted to do that initially as well. However, in the if we are creating a new Message of the IntegrationMimeMessage. This means that when we call setMessageFlagsAndMaybeDeleteMessages and we invoke Message#setFlags the actual implementation differs. If we look into the IMAPMessage#setFlags the implementation looks like

synchronized (getMessageCacheLock()) {
    try {
        IMAPProtocol p = getProtocol();
        checkExpunged(); // Insure that this message is not expunged
        p.storeFlags(getSequenceNumber(), flag, set);
    } catch (ConnectionException cex) {
        throw new FolderClosedException(folder, cex.getMessage());
    } catch (ProtocolException pex) {
        throw new MessagingException(pex.getMessage(), pex);
    }
}

whereas the MimeMessage#setFlags looks like

if (set)
    flags.add(flag);
else
    flags.remove(flag);

I am going to add a comment there to explain why it is done in the way I did it

I’m not sure why does that matter since you call setMessageFlagsAndMaybeDeleteMessages in both cases? So, one local variable for both cases feels right.
Ok, I’ll look into that locally on merge.

@artembilan it matters a lot because what will be invoked in the end is not the same. The behaviour of the setFlags will change due to the fact that there will be different implementations.

Prior to this PR the setFlags was invoked on the original messages received from the folder. The goal of the PR is to still invoke the setFlags on the original messages. However, instead of doing it before copying it, to do it after it has been copied.

I have an idea for a test case. I'll add it to illustrate the problem.

I am still failing to understand why this code is not OK with you:

	private void postProcessFilteredMessages(Message[] filteredMessages) throws MessagingException {
		Message[] messagesToProcess = filteredMessages;

		// Copy messages to cause an eager fetch
		if (this.headerMapper == null && (this.autoCloseFolder || this.simpleContent)) {
			messagesToProcess = new Message[filteredMessages.length];
			for (int i = 0; i < filteredMessages.length; i++) {
				Message originalMessage = filteredMessages[i];
				messagesToProcess[i] = originalMessage;
				MimeMessage mimeMessage = new IntegrationMimeMessage((MimeMessage) originalMessage);
				filteredMessages[i] = mimeMessage;
			}
		}

		setMessageFlagsAndMaybeDeleteMessages(messagesToProcess);
	}

You see, we still fulfill that messagesToProcess with original messages.
And then call setMessageFlagsAndMaybeDeleteMessages() only against a single source of truth.

Sorry @artembilan, I misunderstood you. I know what you mean now. The messages will be the original messages populated in the loop.

I adjusted the PR. Please correct me if I am wrong.

Looks like just AtomicBoolean will be enough without any extra ctor arg.

You are right. I've adjusted it

I am still failing to understand why this code is not OK with you:

	private void postProcessFilteredMessages(Message[] filteredMessages) throws MessagingException {
		Message[] messagesToProcess = filteredMessages;

		// Copy messages to cause an eager fetch
		if (this.headerMapper == null && (this.autoCloseFolder || this.simpleContent)) {
			messagesToProcess = new Message[filteredMessages.length];
			for (int i = 0; i < filteredMessages.length; i++) {
				Message originalMessage = filteredMessages[i];
				messagesToProcess[i] = originalMessage;
				MimeMessage mimeMessage = new IntegrationMimeMessage((MimeMessage) originalMessage);
				filteredMessages[i] = mimeMessage;
			}
		}

		setMessageFlagsAndMaybeDeleteMessages(messagesToProcess);
	}

You see, we still fulfill that messagesToProcess with original messages.
And then call setMessageFlagsAndMaybeDeleteMessages() only against a single source of truth.

Why do we need to override this if there is that Message.getFlags()?

This is to show that we are calling the setFlags on the original message and not the copied one. The Flags can be shared. The MimeMessage has

flags = source.getFlags();
if (flags == null)    // make sure flags is always set
    flags = new Flags();

So if the source has Flags and we call the setFlags on the filteredMessages then the test will still pass. The goal of the change is to show that the setFlags is called on the original message.

Doesn't look like you validate somehow in the test if message was original or IntegrationMimeMessage.
Therefore this extra precaution is redundant.

There is

assertThat(msg2.getMyTestFlags().contains(Flag.SEEN)).isTrue();

In the test.

But looks like it is exactly the same as previous one:

assertThat(msg2.getFlags().contains(Flag.SEEN)).isTrue();

That's why I believe that is really enough.
Because you are not going to deal with mock messages in the real application.

I added this to validate that the setFlags is called on the original message and not on the IntegrationMimeMessage. Currently, GreenMailUtil#newMimeMessage does not set the flags. However, if you want I can set it up in such case that if the setMessageFlagsAndMaybeDeleteMessages is called on the IntegrationMimeMessage the test would fail.

assertThat(msg2.getFlags().contains(Flag.SEEN)).isTrue();

The test above passes as the Flags are shared. But

assertThat(msg2.getMyTestFlags().contains(Flag.SEEN)).isTrue();

would fail as it has it's own flags. If you think it is too much, I can remove it

OK. You probably can spy() on the GreenMailUtil#newMimeMessage result to verfiy that exactly its setFlags() is called.
It is still look redundant to have our own property when it is there in the super class.

good point with the spy(). I'll add that

I've replaced that with a spy. Hope it is good now.

Doesn't look like you validate somehow in the test if message was original or IntegrationMimeMessage.
Therefore this extra precaution is redundant.

But looks like it is exactly the same as previous one:

assertThat(msg2.getFlags().contains(Flag.SEEN)).isTrue();

That's why I believe that is really enough.
Because you are not going to deal with mock messages in the real application.